settings: Add scroll for settings page #1911
Conversation
|
Thanks for taking care of this! Before we can review this change, it will need a test. Those screenshots and recordings are very helpful, thanks. One remark: there's nothing unreasonable about the large text size — people vary in how sharp their vision is, and for some people that setting is useful by making it possible to read the text on the screen. |
MritunjayTiwari14
force-pushed
the
issue-1904
branch
from
499c590 to
4685d90
Compare
|
Thank you @gnprice for reminding about the test. I have added the test for scrolling in the latest commit. I have limited the screen size to 200x200px to test the scrolling effect as lower down to 100x100px caused some rendering issue while running the test. |
There was a problem hiding this comment.
Thanks! Comments below.
For the commit message:
settings: Add scroll for settings page
This commit replaced Column with
ListView Widget to enable scrolling effect.
- It needs a
Fixes #<issue number>.line, like other commits that fix issues in the tracker. - The sentence in the body isn't needed; it just restates what's already obvious from reading the diff.
- For the summary line, "add scroll" sounds a little odd to me; how about
settings: Make page scrollable.
test/widgets/settings_test.dart
Outdated
| }, variant: TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); | ||
| }); | ||
|
|
||
| group('SettingPageScrollBehavior', () { |
There was a problem hiding this comment.
This string looks like an identifier, like the name of a class or something, but then I don't find anything when I look for its definition. To be less confusing, we should just say "SettingsPage scroll behavior" or similar. (SettingsPage is the name of a class, so it doesn't add confusion to use that name.)
There was a problem hiding this comment.
Also nit: the test groups are now in this order:
- 'ThemeSetting'
- 'BrowserPreference'
- (this group)
- 'VisitFirstUnreadSetting'
- 'MarkReadOnScrollSetting'
To be more organized, let's put the new group before the groups that are about individual settings, instead of in the middle of them.
There was a problem hiding this comment.
Yes, Moving it to the top seems reasonable here.
test/widgets/settings_test.dart
Outdated
| final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty | ||
| ? find.text("Experimental features") | ||
| : find.text("Mark messages as read on scroll"); | ||
| check(lastElementFinder.evaluate().isEmpty).equals(true); |
There was a problem hiding this comment.
nit: how about check(lastElementFinder).findsNothing();
There was a problem hiding this comment.
(Good thought to test that the row isn't visible before scrolling. We want to be sure that the scrolling is actually what causes the row to become visible, rather than some other factor, which could arise in the future, like the settings being presented in a different order than when we wrote this test.)
test/widgets/settings_test.dart
Outdated
|
|
||
| await tester.scrollUntilVisible(lastElementFinder, 100, | ||
| scrollable: find.byType(Scrollable)); | ||
| check(lastElementFinder.evaluate().isEmpty).equals(false); |
There was a problem hiding this comment.
nit: how about `check(lastElementFinder).findsOne();
There was a problem hiding this comment.
Seems more high level.
test/widgets/settings_test.dart
Outdated
| final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty | ||
| ? find.text("Experimental features") | ||
| : find.text("Mark messages as read on scroll"); |
There was a problem hiding this comment.
| final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty | |
| ? find.text("Experimental features") | |
| : find.text("Mark messages as read on scroll"); | |
| final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty | |
| ? find.text("Experimental features") | |
| : find.text("Mark messages as read on scroll"); |
test/widgets/settings_test.dart
Outdated
| }); | ||
|
|
||
| group('SettingPageScrollBehavior', () { | ||
| testWidgets('scroll settings list when screen size is small', (tester) async { |
There was a problem hiding this comment.
nit:
| testWidgets('scroll settings list when screen size is small', (tester) async { | |
| testWidgets('content is scrollable when taller than a screenful', (tester) async { |
(Helps focus the reader on what the test's purpose is, and acknowledges that the test can still be helpful when the screen is normal-sized, in a future where the list of settings is much longer.)
MritunjayTiwari14
force-pushed
the
issue-1904
branch
from
4685d90 to
08e169b
Compare
|
Thank you @chrisbobbe for taking time to review, all the requested changes and some other nits have been pushed. |
3 participants
Video Recordings
Before
before.mp4
After (Normal)
after.mp4
After (Increased Text Size to unreasonably high in android settings to test edge case)
after_increased_text_size_too_much_in_android_settings_edge_case.mp4
Fixes #1904